Addressing blocking issues from test plan for inferred tuple names #19016

jcouv · 2017-04-26T16:48:58Z

This PR is not ready for review yet. I will add more tests as Neal goes through the test plan today and provides feedback.

Test plan #18606
FYI @gafter

gafter · 2017-04-26T17:48:00Z

I'm going through the test plan today, using the branch you prepared for this PR as a reference. #Resolved

jcouv · 2017-04-26T18:07:49Z

@gafter
Known issues:

VB does not handle GetSymbolInfo on tuple fields properly: Need to implement semantic info APIs for tuple literals in VB #16697
hovering over tuple element shows sub-optimal QuickInfo: Intellisense displays tuple elements as fields when they are actually constructor arguments #18371 (comment)
I will do additional verification on deconstruction-declaration and deconstruction-foreach this morning. I suspect there is a bug there.
#Resolved

gafter · 2017-04-27T22:12:19Z

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs


-            var inferredPositions = inferNames ? tupleNames.SelectAsArray(n => n != null) : default(ImmutableArray<bool>);
            return new BoundTupleLiteral(syntax, tupleNames, inferredPositions, arguments: valuesBuilder.ToImmutableAndFree(), type: type);


This should probably save the positions even if it isn't in the type, however, this is probably not observable until we permit tuple element names on the left of a deconstruction.

gafter · 2017-04-27T22:17:25Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

                            Compilation,
-                            shouldCheckConstraints: false);
+                            shouldCheckConstraints: false,
+                            inferredPositions: inferredPositions);


Perhaps should be language version dependence here.

gafter · 2017-04-27T22:18:18Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

                            Compilation,
-                            shouldCheckConstraints: false);
+                            shouldCheckConstraints: false,
+                            inferredPositions: inferredPositions);
                        return new BoundTupleLiteral(syntax, default(ImmutableArray<string>), default(ImmutableArray<bool>), subExpressions, tupleType);


And needs to be passed here I think.

gafter

LGTM with a few noted suggestions.

gafter · 2017-04-27T22:59:51Z

Sorry, a number of my comments are in #19021

VSadov

LGTM

…port errors on usage

jcouv · 2017-04-28T05:04:08Z

@dotnet-bot test windows_debug_vs-integration_prtest please

jcouv · 2017-04-28T05:20:52Z

@dotnet-bot test windows_release_vs-integration_prtest please

jcouv · 2017-04-28T18:10:44Z

@dotnet/roslyn-compiler Sorry, I forgot to tag the team on this. Let me know.

cston · 2017-04-28T18:18:02Z

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

-            }
+            var uniqueFieldNames = PooledHashSet<string>.GetInstance();
+            RemoveDuplicateInferredTupleNames(namesBuilder, uniqueFieldNames);
+            uniqueFieldNames.Free();


Can uniqueFieldNames be moved to RemoveDuplicateInferredTupleNames as an implementation detail?

Unfortunately, no. Two of the usages use a fresh hashset (because they can't have explicit names in those scenarios, for instance in deconstruction), but one needs a hashset that already has explicit names (that's in BindTupleExpression).

cston · 2017-04-28T19:02:24Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

@@ -47,13 +54,15 @@ internal sealed class TupleTypeSymbol : WrappedNamedTypeSymbol
        internal const string TupleTypeName = "ValueTuple";
        internal const string RestFieldName = "Rest";

-        private TupleTypeSymbol(Location locationOpt, NamedTypeSymbol underlyingType, ImmutableArray<Location> elementLocations, ImmutableArray<string> elementNames, ImmutableArray<TypeSymbol> elementTypes)
+        private TupleTypeSymbol(Location locationOpt, NamedTypeSymbol underlyingType, ImmutableArray<Location> elementLocations,
+            ImmutableArray<string> elementNames, ImmutableArray<TypeSymbol> elementTypes, ImmutableArray<bool> inferredNamesPositions)


Is there a reason to call the parameter inferredNamesPositions here rather than errorPositions as elsewhere?

That's a lapse. I'll fix in next PR.

cston · 2017-04-28T19:24:01Z

src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleFieldSymbol.vb

@@ -210,10 +210,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
        Inherits TupleElementFieldSymbol

        Private _name As String
+        Private _cannotUse As Boolean ' With LanguageVersion 15, we will produce named elements that should not be used


ReadOnly for both fields.

cston · 2017-04-28T19:35:09Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

+        /// If none of the element names were inferred, or inferred names can be used (no tracking necessary), leave as default.
+        /// This information is ignored in type equality and comparison.
+        /// </summary>
+        private readonly ImmutableArray<bool> _errorPositions;


Since the _errorPositions field only applies to use of inferred names at those positions, would it make sense to call this array _cannotUseInferredNames?

cston · 2017-04-28T19:36:37Z

LGTM

jcouv added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 26, 2017

dnfclas added the cla-already-signed label Apr 26, 2017

jcouv force-pushed the tuple-names-complete branch 3 times, most recently from 5ad2722 to 62257a1 Compare April 27, 2017 07:36

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 27, 2017

jcouv force-pushed the tuple-names-complete branch 2 times, most recently from 8b71d5e to dfa1b4b Compare April 27, 2017 19:07

gafter reviewed Apr 27, 2017

View reviewed changes

gafter approved these changes Apr 27, 2017

View reviewed changes

VSadov approved these changes Apr 27, 2017

View reviewed changes

jcouv and others added 10 commits April 27, 2017 21:24

More tests for inferred tuple names

6eb9957

Add test for inferred names in deconstruction-declaration

6e72e0c

C# 7 and VB 15 should understand inferred tuple element names, but re…

422c269

…port errors on usage

Names inferred in deconstruction affect refactorings

a0084a3

Move language version check to caller

a610f99

Use MessageID for feature availability check

c8f925c

Fix refactoring test

60d158f

Add tests from discussion with Aleksey

47121ae

Public API should accept a SyntaxNode instead of ExpressionSyntax

151e5c3

Address PR feedback from Neal

5176e5d

jcouv force-pushed the tuple-names-complete branch from 563e43f to 5176e5d Compare April 28, 2017 04:26

jcouv merged commit 5734799 into dotnet:features/tuple-names Apr 28, 2017

cston reviewed Apr 28, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing blocking issues from test plan for inferred tuple names #19016

Addressing blocking issues from test plan for inferred tuple names #19016

jcouv commented Apr 26, 2017

gafter commented Apr 26, 2017 •

edited by jcouv

Loading

jcouv commented Apr 26, 2017 •

edited

Loading

gafter Apr 27, 2017

gafter Apr 27, 2017

gafter Apr 27, 2017

gafter left a comment

gafter commented Apr 27, 2017

VSadov left a comment

jcouv commented Apr 28, 2017

jcouv commented Apr 28, 2017

jcouv commented Apr 28, 2017

cston Apr 28, 2017

jcouv Apr 28, 2017

cston Apr 28, 2017

jcouv Apr 28, 2017

cston Apr 28, 2017

cston Apr 28, 2017

cston commented Apr 28, 2017


		var inferredPositions = inferNames ? tupleNames.SelectAsArray(n => n != null) : default(ImmutableArray<bool>);
		return new BoundTupleLiteral(syntax, tupleNames, inferredPositions, arguments: valuesBuilder.ToImmutableAndFree(), type: type);

Addressing blocking issues from test plan for inferred tuple names #19016

Addressing blocking issues from test plan for inferred tuple names #19016

Conversation

jcouv commented Apr 26, 2017

gafter commented Apr 26, 2017 • edited by jcouv Loading

jcouv commented Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

gafter commented Apr 27, 2017

VSadov left a comment

Choose a reason for hiding this comment

jcouv commented Apr 28, 2017

jcouv commented Apr 28, 2017

jcouv commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Apr 28, 2017

gafter commented Apr 26, 2017 •

edited by jcouv

Loading

jcouv commented Apr 26, 2017 •

edited

Loading